Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2 ➡️ 3 #136

Merged
merged 157 commits into from
May 18, 2020
Merged

2 ➡️ 3 #136

merged 157 commits into from
May 18, 2020

Conversation

chapulina
Copy link
Contributor

Merge ign-gazebo2 forward to ign-gazebo3.

@azeey, I had to make some changes to SdfGenerator_TEST on 9ecee15 so that the worlds would load with SDFormat 9. Mind checking if that's the best way to fix those tests?

@iche033 , I have 2 tests failing locally, INTEGRATION_gpu_lidar and INTEGRATION_sensors_system, both at the destruction of GpuLidarSensor with the backtrace below. Have you seen this before and know why it would be failing on this branch? If not, I can dig into it.

Backtrace
Thread 1 "INTEGRATION_sen" received signal SIGSEGV, Segmentation fault.
0x00007fffc1c8d969 in ?? () from /usr/lib/x86_64-linux-gnu/libGLdispatch.so.0
(gdb) bt
#0  0x00007fffc1c8d969 in ?? () from /usr/lib/x86_64-linux-gnu/libGLdispatch.so.0
#1  0x00007fffb616f4c8 in Ogre::GL3PlusFrameBufferObject::~GL3PlusFrameBufferObject() ()
   from /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/RenderSystem_GL3Plus.so
#2  0x00007fffb616eaf2 in ?? () from /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/RenderSystem_GL3Plus.so
#3  0x00007fffb619527a in Ogre::v1::GL3PlusTextureBuffer::~GL3PlusTextureBuffer() ()
   from /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/RenderSystem_GL3Plus.so
#4  0x00007fffb61952b9 in Ogre::v1::GL3PlusTextureBuffer::~GL3PlusTextureBuffer() ()
   from /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/RenderSystem_GL3Plus.so
#5  0x00007fffb61921eb in Ogre::GL3PlusTexture::freeInternalResourcesImpl() ()
   from /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/RenderSystem_GL3Plus.so
#6  0x00007fffc2c0e57d in Ogre::Texture::freeInternalResources() () from /usr/lib/x86_64-linux-gnu/libOgreMain.so.2.1.0
#7  0x00007fffb61922f9 in Ogre::GL3PlusTexture::~GL3PlusTexture() () from /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/RenderSystem_GL3Plus.so
#8  0x00007fffb6192429 in Ogre::GL3PlusTexture::~GL3PlusTexture() () from /usr/lib/x86_64-linux-gnu/OGRE-2.1/OGRE/RenderSystem_GL3Plus.so
#9  0x00007fffc84d2fd2 in Ogre::SharedPtr<Ogre::Texture>::destroy (this=0x7fffc4e902a8) at /usr/include/OGRE-2.1/OgreSharedPtr.h:340
#10 Ogre::SharedPtr<Ogre::Texture>::release (this=0x7fffc4e902a8) at /usr/include/OGRE-2.1/OgreSharedPtr.h:329
#11 0x00007fffc84e4ca9 in Ogre::SharedPtr<Ogre::Texture>::reset (this=<optimized out>) at /usr/include/OGRE-2.1/OgreSharedPtr.h:313
#12 ignition::rendering::v3::Ogre2GpuRays::Destroy() () at /home/chapulina/citadel_ws/src/ign-rendering/ogre2/src/Ogre2GpuRays.cc:202
#13 0x00007fffc85003e3 in ignition::rendering::v3::BaseStore<ignition::rendering::v3::Sensor, ignition::rendering::v3::Ogre2Sensor>::Destroy
Impl (this=<optimized out>, _iter=...) at /home/chapulina/citadel_ws/src/ign-rendering/include/ignition/rendering/base/BaseStorage.hh:953
#14 0x00007fffc850012f in ignition::rendering::v3::BaseStore<ignition::rendering::v3::Sensor, ignition::rendering::v3::Ogre2Sensor>::Destroy
 (this=0x7fffc494be20, _object=...) at /usr/include/c++/8/ext/atomicity.h:82
#15 0x00007fffca5e265d in ignition::rendering::v3::BaseScene::DestroySensor(std::shared_ptr<ignition::rendering::v3::Sensor>, bool) ()
    at /usr/include/c++/8/ext/atomicity.h:96
#16 0x00007fffa4bdd4f3 in ignition::sensors::v3::GpuLidarSensor::RemoveGpuRays(std::shared_ptr<ignition::rendering::v3::Scene>) ()
    at /usr/include/c++/8/ext/atomicity.h:96
#17 0x00007fffa4bde0a4 in ignition::sensors::v3::GpuLidarSensor::~GpuLidarSensor (this=0x7fffc49a1700, __in_chrg=<optimized out>)
    at /home/chapulina/citadel_ws/src/ign-sensors/src/GpuLidarSensor.cc:59
#18 0x00007fffa4bde3a9 in ignition::sensors::v3::GpuLidarSensor::~GpuLidarSensor (this=0x7fffc49a1700, __in_chrg=<optimized out>)
    at /home/chapulina/citadel_ws/src/ign-sensors/src/GpuLidarSensor.cc:57
#19 0x00007fffd01c23ee in std::default_delete<ignition::sensors::v3::Sensor>::operator() (this=0x7fffc4e8fc88, __ptr=<optimized out>)
    at /usr/include/c++/8/bits/unique_ptr.h:347
#20 std::unique_ptr<ignition::sensors::v3::Sensor, std::default_delete<ignition::sensors::v3::Sensor> >::~unique_ptr (this=0x7fffc4e8fc88,
    __in_chrg=<optimized out>) at /usr/include/c++/8/bits/unique_ptr.h:274
#21 std::pair<unsigned long const, std::unique_ptr<ignition::sensors::v3::Sensor, std::default_delete<ignition::sensors::v3::Sensor> > >::~p
air (this=0x7fffc4e8fc80, __in_chrg=<optimized out>) at /usr/include/c++/8/bits/stl_pair.h:208
#22 __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<unsigned long const, std::unique_ptr<ignition::sensors::v3::Sensor, std::default_d
elete<ignition::sensors::v3::Sensor> > > > >::destroy<std::pair<unsigned long const, std::unique_ptr<ignition::sensors::v3::Sensor, std::def
ault_delete<ignition::sensors::v3::Sensor> > > > (this=0x555556dcca00, __p=0x7fffc4e8fc80) at /usr/include/c++/8/ext/new_allocator.h:140
#23 std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<unsigned long const, std::unique_ptr<ignition::sensors::v3::Sensor, st
d::default_delete<ignition::sensors::v3::Sensor> > > > > >::destroy<std::pair<unsigned long const, std::unique_ptr<ignition::sensors::v3::Se
nsor, std::default_delete<ignition::sensors::v3::Sensor> > > > (__a=..., __p=0x7fffc4e8fc80) at /usr/include/c++/8/bits/alloc_traits.h:487

Signed-off-by: Louise Poubel <[email protected]>
@iche033
Copy link
Contributor

iche033 commented May 14, 2020

This fixes the tests for me. It makes sure rendering sensors are removed in the rendering thread:

diff --git a/src/systems/sensors/Sensors.cc b/src/systems/sensors/Sensors.cc
index f93f62f1..1607cbfe 100644
--- a/src/systems/sensors/Sensors.cc
+++ b/src/systems/sensors/Sensors.cc
@@ -262,6 +262,10 @@ void SensorsPrivate::RenderThread()
   {
     this->RunOnce();
   }
+
+  for (auto id : this->sensorIds)
+    this->sensorManager.Remove(id);
+
   igndbg << "SensorsPrivate::RenderThread stopped" << std::endl;
 }

Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

This fixes the tests for me.

Thanks, @iche033 ! That did the trick for me locally! Pushed on f33d639. I'll see if it needs to be backported to ign-gazebo2.

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I made a comment on 9ecee15 saying that we can don't have to remove the invalid URIs to fix the test. And a couple of comments below:

src/SdfGenerator.cc Outdated Show resolved Hide resolved
src/SdfGenerator.cc Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor Author

I'll see if it needs to be backported to ign-gazebo2.

I can't reproduce that test failure on ign-gazebo2 so I think we're good.

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #136 into ign-gazebo3 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gazebo3     #136   +/-   ##
============================================
  Coverage        65.67%   65.67%           
============================================
  Files              127      127           
  Lines             6238     6238           
============================================
  Hits              4097     4097           
  Misses            2141     2141           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f5bd7a...7f5bd7a. Read the comment docs.

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

The Ubuntu build is clean and the OSX one has one new failure, INTEGRATION_breadcrumbs. I SSH'ed into that CI machine (glados) and couldn't reproduce the failure. Spurious perhaps? Triggered a new homebrew build: Build Status

@chapulina
Copy link
Contributor Author

Spurious perhaps?

The test passed now. Let's hope it was a spurious failure, and not a new flaky test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants